refactor(cli): render database history through the shared status table renderer by aparajon · Pull Request #1206 · block/schemabot

aparajon · 2026-08-29T16:48:24Z

Why this matters

#1062 gave the status list a real column renderer — optional columns that drop when no row fills them, dash-fill for missing values, width sizing, state coloring — but it was typed to ActiveApplyData, so the history table next to it still hand-rolls its layout with five parallel maxLen width variables and a positional fmt.Printf that must be kept in sync by hand. Every column added to either table means re-deriving alignment logic in a second place, and the two tables had already drifted on a detail (where the color escape closes relative to the column separator). This PR makes the renderer generic over the row type and moves the history table onto it, with no change to what operators see.

What it does

statusColumn and its helpers (retainPopulatedStatusColumns, statusColumnValue, statusColumnWidths) become generic over the row type, and a new writeStatusTable owns the shared header+rows loop, taking a row-state accessor so colored columns work for any row struct. WriteDatabaseHistory now just declares its columns.

Before
  WriteStatusList       ──► statusColumn{value func(ActiveApplyData)}
  │                         └─ inline header/row loop, width sizing, coloring
  WriteDatabaseHistory  ──► maxID/maxEnv/maxState/maxStarted/maxDur
                            └─ positional fmt.Printf layout, own coloring

After
  WriteStatusList       ──► statusListColumns()      ─┐
                                                      ├─► writeStatusTable[Row]
  WriteDatabaseHistory  ──► databaseHistoryColumns() ─┘     ├─ widths: header vs widest cell
                                                            ├─ dash for a value a row is missing
                                                            └─ state-colored cell, escape closed
                                                               before the column separator

Two rendering properties worth calling out:

History output is byte-identical, pinned by exact-bytes tests (fixed clocks, full-escape-sequence expected strings) covering colored states, an unrecognized state, missing timestamps, and the empty-database message.
The status list adopts the history table's escape placement: a colored state cell now closes its ANSI escape before the two-space separator instead of after. That renders identically on any terminal, and TEMPLATES.md regenerates with no diff.

How it moves us toward the northstar

Operator CLI surfaces are growing tables faster than they're growing layouts — deployment-filtered lists, remote-handle columns, history. With one shared renderer, the next table (and the next column on an existing one) is a column declaration, not a second alignment implementation, so the fleet CLI keeps one consistent table behavior as it expands.

Opened by Claude (Fable 5).

…e renderer Generalize statusColumn and its helpers over the row type so any CLI table can define columns against its own row struct, add writeStatusTable as the single header+rows renderer, and replace WriteDatabaseHistory's hand-rolled column layout with history-specific statusColumn definitions. History output is byte-identical, pinned by an exact-bytes test; the status list's colored state cell now ends its color escape before the column separator, which renders identically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Refactors the CLI table rendering in templates to reuse a single shared “status table” renderer across multiple outputs, starting with the database history view, while preserving the existing user-facing formatting (including ANSI coloring behavior).

Changes:

Generalizes statusColumn and related helpers over an arbitrary row type and introduces writeStatusTable to own the shared header/row rendering loop (including correct ANSI reset behavior around separators).
Refactors WriteStatusList and WriteDatabaseHistory to render via writeStatusTable, replacing bespoke printing/width tracking in history output.
Adds exact-bytes tests for the database history table output (including unknown-state and missing-timestamp cases) plus an empty-history test.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/cmd/internal/templates/progress.go	Introduces generic status-table primitives (`statusColumn[T]`, `writeStatusTable`) and refactors status list + database history rendering to use them.
pkg/cmd/internal/templates/progress_states_test.go	Adds exact-bytes tests pinning database history table output and the empty-history message.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Kiran01bm · 2026-08-30T20:50:00Z

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1206, 1329361.

Verdict: 3 findings — no blockers; 1 non-blocking (the refactor silently changes a second surface), 2 suggestions.

Non-blocking

The refactor changes WriteStatusList's bytes too, which the PR never mentions. Previously statusCell baked the two-space separator into the cell (fmt.Sprintf("%-*s ", width, value)) and colorFn wrapped the whole thing, so the STATE cell emitted ESC[32mCompleted␣␣␣␣␣␣␣ESC[0m. Now statusCell drops the separator and writeStatusTable prints it after the escape, giving ESC[32mCompletedESC[0m␣␣ — every status-list row with a recognized state changes. The two surfaces disagreed before (history already put the separator outside), and unifying them is the right call, but the PR is titled and described as a history change, adds byte-exact tests only for history, and leaves the surface it actually altered unpinned.

General suggestions

History is not byte-identical as the PR body claims. Routing through statusColumnValue substitutes - for an empty ApplyID, Environment, State or Caller, where the removed hand-rolled Printf emitted a blank padded cell. This is reachable, not theoretical — caller is varchar(255) NOT NULL DEFAULT '' and resolveCaller returns the request caller unchanged when there is no authenticated subject, and both applySource("") and state.Label("") return "". The new dash is the better rendering and matches the status list; the issue is that TestWriteDatabaseHistoryTable only exercises missing timestamps, which already produced - on both sides, so the one genuinely divergent input is untested.
The new shared renderer bakes in byte-width. statusColumnWidths measures with len(), so the moment any cell carries a multi-byte value — a non-ASCII database or environment name, or a state glyph if these tables ever gain one — every column right of it misaligns. ui.VisibleWidth / ui.PadVisible exist for exactly this and already handle emoji-variation sequences; this is the natural moment to adopt them, since the whole point of the change is to make one renderer the shared path. No live bug today: every current cell is ASCII.

The one thing that could have broken, verified

Whether generifying statusColumn silently changed which columns the status list drops. It did not: retainPopulatedStatusColumns and anyStatusRowFillsColumn are the same logic with ActiveApplyData replaced by a type parameter, statusListColumns still calls it, and the optional/unconditional split for EXTERNAL ID is untouched. The history path deliberately bypasses it — WriteDatabaseHistory calls databaseHistoryColumns() directly and none of those columns are optional, so no history column can ever be dropped, matching the old renderer which had no dropping at all. Header-row bytes are also unchanged on both surfaces: the old and new forms both dim the entire row including separators.

Verified correct

Column-width seeds are identical: the old hand-rolled minimums (8/3/5/7/8) are exactly len(header) for APPLY ID / ENV / STATE / STARTED / DURATION.
formatStartedAt("") and formatApplyDuration("", …) already returned - themselves, so those cells are genuinely unchanged.
writeFailedStatusList renders per-apply blocks, not a table, so there is no third surface that should have been unified and was missed.
The last: true column correctly suppresses both the padding and the trailing separator, so no row gains trailing whitespace.
stateColorFunc is foreground-only, so finding 1 is a byte change with no visual difference in a terminal.
go test -count=1 ./pkg/cmd/internal/templates/ passes at this head, and all CI checks are green — consistent with no existing test having pinned the status-list colored bytes.

This review was generated by Claude Code (claude-opus-5).

Copilot AI lite review requested due to automatic review settings August 29, 2026 16:48

Copilot started reviewing on behalf of aparajon August 29, 2026 16:48 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

aparajon marked this pull request as ready for review August 30, 2026 13:46

aparajon requested review from JashLal, Kiran01bm, eeSeeGee, jayjanssen, jemiahw and morgo as code owners August 30, 2026 13:46

Kiran01bm approved these changes Aug 30, 2026

View reviewed changes

Navigation Menu

Uh oh!

refactor(cli): render database history through the shared status table renderer - #1206

refactor(cli): render database history through the shared status table renderer#1206
aparajon wants to merge 1 commit into
mainfrom
armand/history-status-columns

aparajon commented Aug 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Kiran01bm commented Aug 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aparajon commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this matters

What it does

How it moves us toward the northstar

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Kiran01bm commented Aug 30, 2026

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aparajon commented Aug 29, 2026 •

edited

Loading